-
Notifications
You must be signed in to change notification settings - Fork 1.4k
ENH: support additional dtypes in pad_nd #8672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
ENH: support additional dtypes in pad_nd #8672
Conversation
Prefer the PyTorch padding backend when supported and safely fall back to NumPy on error. Add unit tests to validate backend selection and ensure output dtype is preserved. Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
📝 WalkthroughWalkthroughpad_nd now unconditionally attempts the PyTorch padding implementation for modes {"constant", "reflect", "edge", "replicate", "wrap", "circular"}, removing the previous dtype-based gating. If a non-constant mode is passed with a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/transforms/croppad/functional.py (1)
99-110: Critical: NotImplementedError not caught by except clause.Line 103 catches
(ValueError, TypeError, RuntimeError)but line 104 checksisinstance(err, NotImplementedError). NotImplementedError would propagate uncaught, breaking the fallback mechanism. The test attest_pad_falls_back_to_np_if_pt_raisesexpects this fallback but would fail in real execution.🔎 Proposed fix
- except (ValueError, TypeError, RuntimeError) as err: + except (ValueError, TypeError, RuntimeError, NotImplementedError) as err: if isinstance(err, NotImplementedError) or any( k in str(err) for k in ("supported", "unexpected keyword", "implemented", "value") ):
🧹 Nitpick comments (1)
tests/transforms/croppad/test_pad_nd_dtypes.py (1)
49-58: Consider testing additional modes.Current tests only cover "constant" mode. The updated code supports {"reflect", "edge", "replicate", "wrap", "circular"} via PyTorch. Testing dtype preservation across these modes would strengthen coverage.
Optional enhancement
@pytest.mark.parametrize( "mode", ["constant", "reflect", "replicate"] ) @pytest.mark.parametrize( "dtype", [torch.bool, torch.int8, torch.float32] ) def test_pad_modes_with_dtypes(mode, dtype): """Test that pad_nd handles various modes and dtypes correctly.""" img = torch.ones((1, 4, 4), dtype=dtype) to_pad = [(0, 0), (1, 1), (2, 2)] out = pad_nd(img, to_pad, mode=mode, value=0) assert out.shape == (1, 6, 8) assert out.dtype == img.dtype
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/croppad/functional.pytests/transforms/croppad/test_pad_nd_dtypes.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/croppad/functional.pytests/transforms/croppad/test_pad_nd_dtypes.py
🧬 Code graph analysis (2)
monai/transforms/croppad/functional.py (1)
monai/transforms/utils_pytorch_numpy_unification.py (1)
mode(426-438)
tests/transforms/croppad/test_pad_nd_dtypes.py (1)
monai/transforms/croppad/functional.py (3)
pad_nd(73-110)_pt_pad(59-70)_np_pad(45-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: build-docs
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: packaging
- GitHub Check: min-dep-py3 (3.9)
Strip value for non-constant modes for both PyTorch and NumPy backends and ensure reliable fallback behavior. Update tests to cover multiple padding modes and dtype preservation. Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/transforms/croppad/test_pad_nd_dtypes.py (4)
1-20: Module docstring and imports look good.Past review comments about missing module docstring have been addressed.
22-31: Test correctly validates PyTorch backend usage and dtype preservation.Past docstring feedback addressed.
33-45: Fallback test is well-structured.Mocking
_pt_padto raiseNotImplementedErrorand verifying_np_padis called covers the new fallback path. Past docstring feedback addressed.
47-57: Good dtype coverage.Tests bool, int8/16/32/64, uint8, float32. Past docstring feedback addressed.
🧹 Nitpick comments (2)
monai/transforms/croppad/functional.py (1)
99-112: Minor formatting: double spaces before**call_kwargs.Lines 107 and 112 have two spaces before
**call_kwargs.🔎 Fix spacing
except NotImplementedError: - return _np_pad(img, pad_width=to_pad, mode=mode, **call_kwargs) + return _np_pad(img, pad_width=to_pad, mode=mode, **call_kwargs) except (ValueError, TypeError, RuntimeError) as err: if any( k in str(err) for k in ("supported", "unexpected keyword", "implemented", "value") ): - return _np_pad(img, pad_width=to_pad, mode=mode, **call_kwargs) + return _np_pad(img, pad_width=to_pad, mode=mode, **call_kwargs)tests/transforms/croppad/test_pad_nd_dtypes.py (1)
59-69: Consider omittingvalue=0for non-constant modes.Line 66 passes
value=0for all modes, but "reflect" and "replicate" don't use this parameter. While the implementation strips it, omitting it in tests makes the intent clearer.🔎 Suggested change
- out = pad_nd(img, to_pad, mode=mode, value=0) + kwargs = {"value": 0} if mode == "constant" else {} + out = pad_nd(img, to_pad, mode=mode, **kwargs)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/croppad/functional.pytests/transforms/croppad/test_pad_nd_dtypes.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/croppad/functional.pytests/transforms/croppad/test_pad_nd_dtypes.py
🧬 Code graph analysis (1)
monai/transforms/croppad/functional.py (1)
monai/transforms/utils_pytorch_numpy_unification.py (1)
mode(426-438)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (macOS-latest)
🔇 Additional comments (1)
monai/transforms/croppad/functional.py (1)
102-105: Logic looks correct for strippingvaluein non-constant modes.Copying kwargs and removing "value" when mode != "constant" prevents passing unsupported arguments to reflect/replicate/etc. modes.
Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/transforms/croppad/test_pad_nd_dtypes.py (4)
15-20: Import ordering could follow PEP8 more strictly.Group imports: standard library, third-party, local. Consider:
from __future__ import annotations + from unittest.mock import Mock, patch + import pytest import torch + import monai.transforms.croppad.functional as F from monai.transforms.croppad.functional import pad_nd
22-31: Consider verifying output shape and values.Test confirms backend selection and dtype preservation but doesn't check padding correctness. Add assertions:
assert out.shape == (1, 6, 8) assert out[0, 1, 2].item() == True # verify padded region if needed
33-45: Expand fallback testing to cover other exception types.The implementation also catches
ValueError,TypeError, andRuntimeErrorwith message matching. Test these paths:🔎 Additional test cases
@pytest.mark.parametrize("error_type,message", [ (ValueError, "not supported"), (TypeError, "unexpected keyword argument"), (RuntimeError, "not implemented"), ]) def test_pad_falls_back_on_other_errors(error_type, message): """Test fallback when PyTorch raises ValueError/TypeError/RuntimeError.""" img = torch.ones((1, 4, 4), dtype=torch.bool) to_pad = [(0, 0), (1, 1), (2, 2)] with ( patch.object(F, "_pt_pad", new=Mock(side_effect=error_type(message))), patch.object(F, "_np_pad", wraps=F._np_pad) as mock_np, ): out = pad_nd(img, to_pad, mode="constant", value=0) assert mock_np.called assert out.dtype == img.dtype
59-70: LGTM. Consider expanding mode coverage.Correctly handles
valuekwarg for constant mode only. For more comprehensive testing, add modes like"edge","wrap","circular".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/croppad/functional.pytests/transforms/croppad/test_pad_nd_dtypes.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/transforms/croppad/functional.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
tests/transforms/croppad/test_pad_nd_dtypes.py
🧬 Code graph analysis (1)
tests/transforms/croppad/test_pad_nd_dtypes.py (1)
monai/transforms/croppad/functional.py (3)
pad_nd(73-119)_pt_pad(59-70)_np_pad(45-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (mypy)
🔇 Additional comments (1)
tests/transforms/croppad/test_pad_nd_dtypes.py (1)
47-57: LGTM.Good dtype coverage. Shape and dtype assertions are appropriate for this test.
ericspod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @shubham-61969 thanks for the contribution. I think the change itself is fine with a few comments, the tests do need to be reformulated with unittest in particular. I think the previous implementation with its limitations was a result of older PyTorch versions so it's good to get this fix in. Please have a look again and then we can rereview.
| if mode != "constant": | ||
| call_kwargs.pop("value", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this condition isn't present, the effect of providing a value argument and not use "constant" mode is to raise an exception in the pad routine used, I think this is intended behaviour. Here if the value argument is removed this silently allows unintended arguments to be ignored, it's better to raise an exception instead.
- Raise an explicit error when �alue is provided with non-constant modes. - Rewrite tests using unittest + parameterized to match MONAI style. Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
for more information, see https://pre-commit.ci
Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
…61969/MONAI into 7842-pad-nd-more-dtypes
|
Hi @ericspod , thanks for the feedback. I’ve updated the implementation to explicitly raise an error when value is provided with modes other than "constant", so the original contract is preserved. I also reworked the tests to use unittest + parameterized and aligned them with MONAI’s existing test style. All GitHub CI checks are now passing. Could you please take another look? If there are any further changes you’d like, I’m happy to address them otherwise I believe this should be ready to merge. |
ericspod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now, I had some minor comments only. We'll try to get this merge shortly when other things have been resolved.
Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
monai/transforms/croppad/functional.py (2)
95-113: Guarded ValueError is swallowed by fallback logic.The new
ValueErroris raised inside thetry, then caught and treated as a fallback because"value"matches the substring filter. This bypasses the intended contract and can allowvaluefor non-constant modes (especially callable modes), or change the error path/message.🐛 Proposed fix
- try: - _pad = _np_pad - if mode in {"constant", "reflect", "edge", "replicate", "wrap", "circular"}: - # Try PyTorch pad for these modes; fallback to NumPy on error. - _pad = _pt_pad - if mode != "constant" and "value" in kwargs: - raise ValueError("'value' argument is only valid when mode='constant'") - return _pad(img, pad_width=to_pad, mode=mode, **kwargs) + if mode != "constant" and "value" in kwargs: + raise ValueError("'value' argument is only valid when mode='constant'") + try: + _pad = _np_pad + if mode in {"constant", "reflect", "edge", "replicate", "wrap", "circular"}: + # Try PyTorch pad for these modes; fallback to NumPy on error. + _pad = _pt_pad + return _pad(img, pad_width=to_pad, mode=mode, **kwargs)
73-93: Document the new ValueError in the docstring.The docstring doesn’t describe the new exception. Add a
Raisessection per the project’s docstring rules.📝 Suggested docstring update
Args: img: data to be transformed, assuming `img` is channel-first and padding doesn't apply to the channel dim. to_pad: the amount to be padded in each dimension [(low_H, high_H), (low_W, high_W), ...]. default to `self.to_pad`. mode: available modes: (Numpy) {``"constant"``, ``"edge"``, ``"linear_ramp"``, ``"maximum"``, ``"mean"``, ``"median"``, ``"minimum"``, ``"reflect"``, ``"symmetric"``, ``"wrap"``, ``"empty"``} (PyTorch) {``"constant"``, ``"reflect"``, ``"replicate"``, ``"circular"``}. One of the listed string values or a user supplied function. Defaults to ``"constant"``. See also: https://numpy.org/doc/stable/reference/generated/numpy.pad.html https://pytorch.org/docs/stable/generated/torch.nn.functional.pad.html kwargs: other arguments for the `np.pad` or `torch.pad` function. note that `np.pad` treats channel dimension as the first dimension. + Raises: + ValueError: If `value` is provided when `mode` is not ``"constant"``.As per coding guidelines, ...
🤖 Fix all issues with AI agents
In `@tests/transforms/croppad/test_pad_nd_dtypes.py`:
- Around line 71-79: The test docstring for
test_pad_dtype_no_error_and_dtype_preserved is missing a Google-style Args
section documenting the parameter `dtype`; update the docstring of
test_pad_dtype_no_error_and_dtype_preserved to include an Args block that
describes the `dtype` parameter (e.g., "dtype: torch dtype to test that pad_nd
preserves input dtype") so the parameterized test using DTYPES is properly
documented; keep the description concise and consistent with other test
docstrings and retain references to pad_nd, DTYPES, and the asserted behavior.
- Around line 81-91: Update the docstring for the test method
test_pad_multiple_modes_dtype_preserved to include a Google-style Args section
documenting the two parameters `mode` and `dtype`; keep the existing one-line
summary and add an Args block that briefly describes `mode` (padding mode under
test) and `dtype` (tensor dtype used for the input image) so the parameterized
test's inputs are documented.
Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
Prefer the PyTorch padding backend when supported and safely fall back
to NumPy on error. Add unit tests to validate backend selection and
ensure output dtype is preserved.
Fixes #7842
Description
This pull request relaxes dtype restrictions in
pad_ndand prefersthe PyTorch padding backend when supported, with a safe fallback to
NumPy on error. This enables support for additional dtypes (e.g. bool)
that are already handled correctly by recent PyTorch versions.
Unit tests are added to validate backend selection and ensure dtype
preservation.
Types of changes